Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tighten up IterativeFinder async close behavior (DHT iterator continues after consumer breaks out of it) #3599

Merged
merged 9 commits into from
May 23, 2022

Conversation

moodyjon
Copy link
Contributor

See: #3504

Script:
scratch.py.txt

Script output:
scratch_1.txt

Initially there is a lot of background activity from IterativeNodeFinder during what I presume is startup.
In steady state there is little activity after each "PAUSE---". Usually just stuff related to cancellation, if anything.

Eventually, I hope the "async with aclosing(...)" feature can be used:
https://docs.python.org/3.10/library/contextlib.html?highlight=aclosing#contextlib.aclosing

@eukreign eukreign assigned shyba and unassigned eukreign Apr 27, 2022
@kauffj kauffj requested a review from shyba April 27, 2022 15:11
@shyba
Copy link
Member

shyba commented Apr 28, 2022

Hello, thank you for looking into it!

Since aclosing is introduced in python 3.10, I think it would be better to add the snippet that they show as equivalent to utils.py:

from contextlib import asynccontextmanager

@asynccontextmanager
async def aclosing(thing):
    try:
        yield thing
    finally:
        await thing.aclose()

Then, we can use it from utils until we support 3.10, changing the import to the built-in one.

I modified the snippet to test that:

import asyncio
import inspect
from contextlib import asynccontextmanager

@asynccontextmanager
async def aclosing(thing):
    try:
        yield thing
    finally:
        close = thing.aclose()
        if inspect.isawaitable(close):
            await close

from lbry.dht.constants import generate_id
from lbry.dht.node import Node
from lbry.dht.peer import PeerManager
from lbry.extras.daemon.storage import SQLiteStorage
from lbry.conf import Config
import logging
logging.basicConfig(level=logging.DEBUG, format="%(asctime)s %(levelname)-4s %(name)s:%(lineno)d: %(message)s")
log = logging.getLogger(__name__)


async def do_it():
    loop = asyncio.get_event_loop()
    #loop.set_debug(True)
    conf = Config()
    print(str(conf))
    storage = SQLiteStorage(conf, ":memory:", loop, loop.time)
    await storage.open()
    node = Node(
        loop, PeerManager(loop), generate_id(), 4444, 4444, 4444, None,
        storage=storage
    )
    node.start(interface='0.0.0.0', known_node_urls=conf.known_dht_nodes)
    await asyncio.sleep(10)
    key = bytes.fromhex('2ecce30223bc2808bd10cf8e18f1cccecedf9fd35fcc370b9d15ad7473a59d3dc16e85a8f08a1891e668608d1660b8c7')
    while True:
        async with aclosing(node.get_iterative_value_finder(key=key, max_results=1)) as values:
            i = 0
            print('SEARCH---')
            async for _ in values:
                print(f'{i}: {_}')
                i += 1
                break
        print('PAUSE---')
        await asyncio.sleep(10)
    await asyncio.wait(asyncio.all_tasks())

if __name__ == '__main__':
    asyncio.run(do_it())

Doing it this way, the pause and search messages clean, like

PAUSE---
SEARCH---

This looks cleaner than implementing the finally->aclose and other changes on every call and we would be able to use the built in version when it is out.

@moodyjon
Copy link
Contributor Author

moodyjon commented Apr 29, 2022

Implemented an aclosing() in lbry/utils.py as suggested.

Copy link
Member

@shyba shyba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the quick reply!
Aside from trapping the CancelledError, I think this looks great but we need to separate the change in behavior from the iterator bugfix to ease testing and changelog.

lbry/dht/protocol/iterative_find.py Outdated Show resolved Hide resolved
lbry/dht/protocol/iterative_find.py Outdated Show resolved Hide resolved
lbry/dht/protocol/iterative_find.py Outdated Show resolved Hide resolved
@lbry-bot lbry-bot assigned eukreign and unassigned shyba May 2, 2022
@moodyjon moodyjon requested a review from shyba May 3, 2022 20:44
@lbry-bot lbry-bot assigned shyba and unassigned eukreign May 3, 2022
@moodyjon
Copy link
Contributor Author

moodyjon commented May 3, 2022

I think I have addressed the 3 comments. The IterativeFinder is mostly back to the original logic with no generator function stuff or output limiting based on max_results. Let me know if there are more comments or a request to squash changes.

@lbry-bot lbry-bot assigned eukreign and unassigned shyba May 6, 2022
@moodyjon
Copy link
Contributor Author

I've seen this failure in my runs as well. Seems like the upper bound needs a bump analogous to:

fd73412

File "/home/runner/work/lbry-sdk/lbry-sdk/tests/integration/other/test_exchange_rate_manager.py", line 30, in test_exchange_rate_manager
123
self.assertLessEqual(lbc, 20_000)
124
AssertionError: Decimal('20004.00080016') not less than or equal to 20000
125

@moodyjon
Copy link
Contributor Author

Argggggh :-/

test_exchange_rate_manager.py still failing, but due to server being down:

Hotbit is under maintenance...
...
Dear Respected Hotbit Users, Hotbit will be under maintenance from 1:30 AM to 1:30 PM UTC on 17th May to optimize the servers' performance. During the maintenance, the service will be unavailable. We apologize for any inconvenience caused!

@eukreign
Copy link
Member

eukreign commented May 18, 2022

@moodyjon @shyba what's the status of this PR? (it needs a rebase to get some test run fixes from master)

@eukreign eukreign assigned moodyjon and unassigned eukreign May 18, 2022
@shyba
Copy link
Member

shyba commented May 18, 2022

@moodyjon Can you please rebase on master instead of merging? Using rebase you can also remove the merge commits, making it a cleaner history

@coveralls
Copy link

coveralls commented May 18, 2022

Coverage Status

Coverage increased (+0.01%) to 57.379% when pulling e5e9873 on moodyjon:async-for-pr3504 into 5c708e1 on lbryio:master.

@moodyjon
Copy link
Contributor Author

@moodyjon @shyba what's the status of this PR? (it needs a rebase to get some test run fixes from master)

I had been observing/merging the test changes. Added one more test change yesterday: f91bf8f

@moodyjon Can you please rebase on master instead of merging? Using rebase you can also remove the merge commits, making it a cleaner history

OK, I think I have rebased correctly.

@moodyjon moodyjon removed their assignment May 18, 2022
…ait after task cancel().

Also make IterativeFinder a proper AsyncGenerator. This gives it an offically recognized aclose() method and could help with clean finalization.
@moodyjon
Copy link
Contributor Author

Removed exchange rate test bump of upper limit. It was changed in 5c708e1

@shyba shyba merged commit 1d95eb1 into lbryio:master May 23, 2022
@shyba
Copy link
Member

shyba commented May 23, 2022

@moodyjon thank you very much!

@eukreign eukreign added type: improvement Existing (or partially existing) functionality needs to be changed area: DHT labels Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: DHT type: improvement Existing (or partially existing) functionality needs to be changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants